[http-client-csharp] Preserve shipped date-time property names - #11773

Open in StackBlitz

npm i https://pkg.pr.new/@typespec/http-client-csharp@11773

commit: 5b0a731

No changes needing a change description found.

Full Azure-branded DPG regeneration completed successfully with the latest changes: all 47 libraries regenerated and there are no resulting SDK file diffs.

Draft regeneration PR: Azure/azure-sdk-for-net#62499

- by copilot

Thanks for the re-review and mutation testing. Addressed the two suggested follow-ups in 788eaab74:

• activeProperties now uses ReferenceEqualityComparer.Instance, making provider-instance identity explicit even if a derived PropertyProvider later overrides equality.
• added the directly named TestDateTimeNameAlreadyEndingOnDoesNotUseHistoricalStemMatch and an explanatory comment for the spec-side On guard.

I also renamed _nounToVerbMap to _nounToVerbRules. The exact and compound passes remain separate so exact matches always win before any potentially overlapping suffix rule.

For the declaration-order report from Copilot, I reversed the GA fixture to declare StartsOn before StartOn. TestPropertyNameDoesNotReuseCanonicalNameClaimedBySibling still passes because the current non-deferred lookup evaluates the ownership guard inside the historical predicate and continues to the available StartOn; the reported candidate-caching behavior belonged to the reverted reconciliation prototype.

The focused property/customization/name suite passes (254 tests).

- by copilot

Addendum — my re-review above landed ten seconds after 83d94f97f was pushed, so it was written against a3572f466 and the two follow-ups in it are still open. Here is the review of b7b952eb8 + the revert.

b7b952eb8 — first exact spec mapping

Full suite at 83d94f97f: Failed: 0, Passed: 2105, no build warnings.

Mutation-tested both new guards, reverting one at a time and re-running the whole suite:

Both covered, and the test is a good one — asserting the surviving property and that both wire names remain on the canonical view catches the over-suppression directly rather than by proxy.

I traced the else specifically, since "stop populating this map on one path" is the kind of change that tends to lose something quietly. The only caller supplying knownSpecProperties is CanonicalTypeProvider.BuildProperties, which passes the generated provider's complete map; the entries the removed rebuild would have added come from generatedProperties, whose spec names are a subset of that map because it was built pre-filter. So nothing is dropped and the comment is accurate. It also composes correctly with the activeProperties guard from a3572f466: the map's values are the same PropertyProvider instances as in propertiesToFilter, so filtered-out properties remain in the map but fail the reference check, which is the intended outcome.

Making AddExactName's per-pass first-wins mirror _exactSpecPropertyNames in CanonicalTypeProvider is nice — the two places that resolve exact-vs-alias precedence now read the same way.

The revert

Reverting 3b5aaa62a was the right call, and #11783 is the right home for it. A change that reaches into property-build, constructor, serialization, and visitor lifecycle does not belong in a back-compat naming fix, and validating it against the real management-plane flattening path is exactly the prerequisite.

I verified the revert is clean rather than taking it on trust: PropertyProvider.cs and PropertyProviderTests.cs are byte-identical between a3572f466 and 83d94f97f, and neither prototype test-data directory survives in the tree. Since b7b952eb8 landed between the prototype and its revert and touched TypeProvider.cs, that was worth confirming.

Still open from the re-review above

Neither is a blocker; restating so they don't get lost to the comment-ordering collision:

1. new HashSet<PropertyProvider>(propertiesToFilter) depends on reference equality, which holds today but is not enforced — PropertyProvider is not sealed, so a derived provider overriding Equals/GetHashCode would silently turn the stale-alias filter into a value match with no test failing. ReferenceEqualityComparer.Instance would pin it.
2. The spec-side !identifierName.EndsWith("On", StringComparison.Ordinal) guard is covered only by ExactSpecNameTakesPrecedenceOverCanonicalAliasForCustomization, a test named for a different concern, and the inline comment explains only the contract-side On requirement.

Plus the two cosmetic notes (_nounToVerbMap is now an array, and _ = Properties; re-entrancy).

State of the PR from my side: the core change is sound, every behavioral guard I could identify is individually covered by a test that fails without it, and the full 2105-test suite is green at 83d94f97f.

--generated by Copilot

Addressed the remaining cosmetic notes in 95cb3cc6b:

• removed the defensive _ = Properties from GeneratedPropertiesBySpecName, avoiding the newly exposed re-entrancy path;
• made CanonicalTypeProvider.BuildProperties explicitly build Properties first, capture the populated map immediately afterward, and reuse that snapshot;
• retained the two-pass noun/verb lookup but documented why: all exact matches must win before any potentially overlapping compound suffix rule.

The earlier 788eaab74 commit also pinned active-property membership to ReferenceEqualityComparer.Instance, added direct coverage for the spec-side On guard, and renamed the array to _nounToVerbRules.

The focused suite remains green at 254 tests.

- by copilot

regen: Azure/azure-sdk-for-net#62530

Re-reviewed at 5b0a7314. As before I verified everything in a local worktree rather than reading the diff, and mutation-tested each new guard by removing it one at a time and re-running the suite.

Verification

• Full suite at 5b0a7314: 2106/2106, no warnings.
• Repeated 5x to check stability: 5/5 green. Merge base 806e1ab8 also 5/5 green at 2079/2079.
• The second revert is exactly clean: git diff 95cb3cc6b9 5b0a7314 is empty, so no prototype code or orphaned test data was left behind.

Both prior follow-ups addressed

1. ReferenceEqualityComparer.Instance on the activeProperties set ✓
2. A directly-named test for the spec-side On guard (TestDateTimeNameAlreadyEndingOnDoesNotUseHistoricalStemMatch) ✓

I also noticed Reset() now clears _generatedPropertiesBySpecName. That addresses the stale-map problem more fundamentally than the filtering-time guard alone.

Mutation results

The fixture reversal is a genuine strengthening

Reversing TestPropertyNameDoesNotReuseCanonicalNameClaimedBySibling/TestModel.cs to declare StartsOn before StartOn is not cosmetic. I ran the differential with IsClaimedBySiblingProperty removed from the stem lookup:

• original order (StartOn first) → test still passes
• reversed order (StartsOn first) → test fails

Because the lookup uses FirstOrDefault in declaration order, the old order let the correct answer be found before the guard was ever consulted. Reversed, StartsOn has to be actively rejected first. The guard is load-bearing in that test now, and it wasn't before.

ReferenceEqualityComparer — uncovered, and currently a no-op

Removing it fails nothing. PropertyProvider overrides neither Equals nor GetHashCode, so EqualityComparer<PropertyProvider>.Default is already reference equality. I still think it's worth keeping as executable documentation of the intent, but flagging honestly that it constrains nothing today: if someone later gives PropertyProvider value equality, this line is what preserves correctness, and no test would catch its removal.

The _ = Properties; round-trip — my error

I owe you a correction. My re-entrancy note on _ = Properties; was explicitly hypothetical and labelled "not a regression" — no caller reads the map mid-build. It nonetheless cost a real safeguard, and the code is now back to the unenforced ordering dependency I originally raised as item 4.

The call site in CanonicalTypeProvider.BuildProperties does the right thing explicitly and the XML doc states the contract, so this is defensible as-is. If you want enforcement at zero re-entrancy risk:

internal IReadOnlyDictionary<string, PropertyProvider> GeneratedPropertiesBySpecName
{
    get
    {
        Debug.Assert(_properties is not null, "Properties must be built before reading this map.");
        return _generatedPropertiesBySpecName;
    }
}

Entirely your call — a documented contract with a single compliant caller is a reasonable place to stop.

The IPStartOn acronym scenario is handled

On the suppressed bot comment about acronym normalization, I traced it by hand: spec ipStartTime gives stem IPStarts; candidate IPStartOn stems to IPStarts and matches; but sibling ipStartOn emits IPStartOn, so comparing the emitted form rejects it. No duplicate member. TestPropertyNameDoesNotReuseHistoricalNameClaimedByNormalizedSibling covers it.

Nit

The explanation of why the spec-side On guard exists lives in the test file. A one-line comment beside the guard in PropertyProvider.cs would help, since that's where someone would be tempted to remove it.

Not a finding

During mutation runs I saw two one-off failures (CanRemoveMethodWithGenericTypeConstraintParameter, CanChangePropertyAccessibility). Neither reproduced — the first passed on two repeat mutated runs and three isolated runs, and the second occurred under a mutation that is provably a no-op. Both base and head were 5/5 green. I'm recording it only so it isn't mistaken for signal; I attribute it to local machine contention rather than this PR.

LGTM.

--generated by Copilot